It doesn't really matter here since we know sk != NULL, but arguably the correct error check for sk_push is still <= 0 because it can return -1 in OpenSSL 1.1.1 and LibreSSL (for a NULL stack).

I repeat what I mentioned elsewhere: I really wish @ndossche would stop pushing the use of sk_new_reserve() into the ecosystem. It is a really bad API with dangerous semantics that isn't portable across libcryptos for good reason: if you use it to simplify error checking, your code is badly written and should be fixed; otherwise it's just a micro-optimization that isn't worth the extra API call and #ifdef. Stacks are arrays of pointers using exponential scaling (doubling on resize or similar), so the reallocations are neither frequent nor expensive. Stacks very rarely grow into the thousands. So sk_new_reserve() doesn't save more than a handful of reallocations/memcpys (expecially here, where it is about attributes of which there never are more than a handful at the same time).

@botovq jeez, I pushed for the use of reserve like twice or so. I agree you need the push check but I disagree with your opinion about it being an unnecessary API. I'll stop pushing reserve if that makes you happy and if that means you leave me alone rather than apparently follow me around on github.

@ndossche Yes, two of the three uses I know outside of OpenSSL itself...

Just to be clear: I do not follow you around specifically and I think the work prompted by your static analyzer is really great. What I do is I look at downstream changes and point out things that aren't done ideally. But yes, if you stop using the reserve API this will decrease the likelihood of me complaining.